From dc7422b6a11078963bb357e964369317971ab7c2 Mon Sep 17 00:00:00 2001 From: debris Date: Fri, 11 Aug 2017 16:25:19 +0200 Subject: [PATCH] Fixed panic when canonicalizing not-a-base-url, fixes #4394 --- src/bin/git_checkout.rs | 4 +- src/bin/install.rs | 2 +- src/cargo/core/package_id.rs | 2 +- src/cargo/core/package_id_spec.rs | 2 +- src/cargo/core/source.rs | 45 +++++++++++----------- src/cargo/ops/cargo_install.rs | 2 +- src/cargo/ops/registry.rs | 2 +- src/cargo/sources/config.rs | 10 ++--- src/cargo/sources/git/source.rs | 62 +++++++++++++++++++++---------- src/cargo/util/toml/mod.rs | 2 +- tests/resolve.rs | 10 ++--- 11 files changed, 84 insertions(+), 59 deletions(-) diff --git a/src/bin/git_checkout.rs b/src/bin/git_checkout.rs index 4841b5e4e..7dc9c5c98 100644 --- a/src/bin/git_checkout.rs +++ b/src/bin/git_checkout.rs @@ -40,9 +40,9 @@ pub fn execute(options: Options, config: &Config) -> CliResult { let url = url.to_url()?; let reference = GitReference::Branch(reference.clone()); - let source_id = SourceId::for_git(&url, reference); + let source_id = SourceId::for_git(&url, reference)?; - let mut source = GitSource::new(&source_id, config); + let mut source = GitSource::new(&source_id, config)?; source.update()?; diff --git a/src/bin/install.rs b/src/bin/install.rs index 7aaca4d48..12518926c 100644 --- a/src/bin/install.rs +++ b/src/bin/install.rs @@ -136,7 +136,7 @@ pub fn execute(options: Options, config: &Config) -> CliResult { } else { GitReference::Branch("master".to_string()) }; - SourceId::for_git(&url, gitref) + SourceId::for_git(&url, gitref)? } else if let Some(path) = options.flag_path { SourceId::for_path(&config.cwd().join(path))? } else if options.arg_crate.is_empty() { diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index 7035ef5db..321848c91 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -180,7 +180,7 @@ mod tests { #[test] fn invalid_version_handled_nicely() { let loc = CRATES_IO.to_url().unwrap(); - let repo = SourceId::for_registry(&loc); + let repo = SourceId::for_registry(&loc).unwrap(); assert!(PackageId::new("foo", "1.0", &repo).is_err()); assert!(PackageId::new("foo", "1", &repo).is_err()); diff --git a/src/cargo/core/package_id_spec.rs b/src/cargo/core/package_id_spec.rs index c24b48a36..3136e3fd6 100644 --- a/src/cargo/core/package_id_spec.rs +++ b/src/cargo/core/package_id_spec.rs @@ -270,7 +270,7 @@ mod tests { #[test] fn matching() { let url = Url::parse("http://example.com").unwrap(); - let sid = SourceId::for_registry(&url); + let sid = SourceId::for_registry(&url).unwrap(); let foo = PackageId::new("foo", "1.2.3", &sid).unwrap(); let bar = PackageId::new("bar", "1.2.3", &sid).unwrap(); diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index 98730e3f0..5d1347238 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -114,15 +114,16 @@ struct SourceIdInner { } impl SourceId { - fn new(kind: Kind, url: Url) -> SourceId { - SourceId { + fn new(kind: Kind, url: Url) -> CargoResult { + let source_id = SourceId { inner: Arc::new(SourceIdInner { kind: kind, - canonical_url: git::canonicalize_url(&url), + canonical_url: git::canonicalize_url(&url)?, url: url, precise: None, }), - } + }; + Ok(source_id) } /// Parses a source URL and returns the corresponding ID. @@ -158,16 +159,16 @@ impl SourceId { let precise = url.fragment().map(|s| s.to_owned()); url.set_fragment(None); url.set_query(None); - Ok(SourceId::for_git(&url, reference).with_precise(precise)) + Ok(SourceId::for_git(&url, reference)?.with_precise(precise)) }, "registry" => { let url = url.to_url()?; - Ok(SourceId::new(Kind::Registry, url) + Ok(SourceId::new(Kind::Registry, url)? .with_precise(Some("locked".to_string()))) } "path" => { let url = url.to_url()?; - Ok(SourceId::new(Kind::Path, url)) + SourceId::new(Kind::Path, url) } kind => Err(format!("unsupported source protocol: {}", kind).into()) } @@ -180,25 +181,25 @@ impl SourceId { // Pass absolute path pub fn for_path(path: &Path) -> CargoResult { let url = path.to_url()?; - Ok(SourceId::new(Kind::Path, url)) + SourceId::new(Kind::Path, url) } - pub fn for_git(url: &Url, reference: GitReference) -> SourceId { + pub fn for_git(url: &Url, reference: GitReference) -> CargoResult { SourceId::new(Kind::Git(reference), url.clone()) } - pub fn for_registry(url: &Url) -> SourceId { + pub fn for_registry(url: &Url) -> CargoResult { SourceId::new(Kind::Registry, url.clone()) } pub fn for_local_registry(path: &Path) -> CargoResult { let url = path.to_url()?; - Ok(SourceId::new(Kind::LocalRegistry, url)) + SourceId::new(Kind::LocalRegistry, url) } pub fn for_directory(path: &Path) -> CargoResult { let url = path.to_url()?; - Ok(SourceId::new(Kind::Directory, url)) + SourceId::new(Kind::Directory, url) } /// Returns the `SourceId` corresponding to the main repository. @@ -220,7 +221,7 @@ impl SourceId { CRATES_IO }; let url = url.to_url()?; - Ok(SourceId::for_registry(&url)) + SourceId::for_registry(&url) } pub fn url(&self) -> &Url { @@ -241,31 +242,31 @@ impl SourceId { } /// Creates an implementation of `Source` corresponding to this ID. - pub fn load<'a>(&self, config: &'a Config) -> Box { + pub fn load<'a>(&self, config: &'a Config) -> CargoResult> { trace!("loading SourceId; {}", self); match self.inner.kind { - Kind::Git(..) => Box::new(GitSource::new(self, config)), + Kind::Git(..) => Ok(Box::new(GitSource::new(self, config)?)), Kind::Path => { let path = match self.inner.url.to_file_path() { Ok(p) => p, Err(()) => panic!("path sources cannot be remote"), }; - Box::new(PathSource::new(&path, self, config)) + Ok(Box::new(PathSource::new(&path, self, config))) } - Kind::Registry => Box::new(RegistrySource::remote(self, config)), + Kind::Registry => Ok(Box::new(RegistrySource::remote(self, config))), Kind::LocalRegistry => { let path = match self.inner.url.to_file_path() { Ok(p) => p, Err(()) => panic!("path sources cannot be remote"), }; - Box::new(RegistrySource::local(self, &path, config)) + Ok(Box::new(RegistrySource::local(self, &path, config))) } Kind::Directory => { let path = match self.inner.url.to_file_path() { Ok(p) => p, Err(()) => panic!("path sources cannot be remote"), }; - Box::new(DirectorySource::new(&path, self, config)) + Ok(Box::new(DirectorySource::new(&path, self, config))) } } } @@ -575,15 +576,15 @@ mod tests { fn github_sources_equal() { let loc = "https://github.com/foo/bar".to_url().unwrap(); let master = Kind::Git(GitReference::Branch("master".to_string())); - let s1 = SourceId::new(master.clone(), loc); + let s1 = SourceId::new(master.clone(), loc).unwrap(); let loc = "git://github.com/foo/bar".to_url().unwrap(); - let s2 = SourceId::new(master, loc.clone()); + let s2 = SourceId::new(master, loc.clone()).unwrap(); assert_eq!(s1, s2); let foo = Kind::Git(GitReference::Branch("foo".to_string())); - let s3 = SourceId::new(foo, loc); + let s3 = SourceId::new(foo, loc).unwrap(); assert!(s1 != s3); } } diff --git a/src/cargo/ops/cargo_install.rs b/src/cargo/ops/cargo_install.rs index 808192999..3ce46881b 100644 --- a/src/cargo/ops/cargo_install.rs +++ b/src/cargo/ops/cargo_install.rs @@ -133,7 +133,7 @@ fn install_one(root: Filesystem, let config = opts.config; let (pkg, source) = if source_id.is_git() { - select_pkg(GitSource::new(source_id, config), + select_pkg(GitSource::new(source_id, config)?, krate, vers, config, is_first_install, &mut |git| git.read_packages())? } else if source_id.is_path() { diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index ddbf99896..f7153c190 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -195,7 +195,7 @@ pub fn registry(config: &Config, } = registry_configuration(config)?; let token = token.or(token_config); let sid = match index { - Some(index) => SourceId::for_registry(&index.to_url()?), + Some(index) => SourceId::for_registry(&index.to_url()?)?, None => SourceId::crates_io(config)?, }; let api_host = { diff --git a/src/cargo/sources/config.rs b/src/cargo/sources/config.rs index faae80c2a..12e9c07e8 100644 --- a/src/cargo/sources/config.rs +++ b/src/cargo/sources/config.rs @@ -73,7 +73,7 @@ impl<'cfg> SourceConfigMap<'cfg> { debug!("loading: {}", id); let mut name = match self.id2name.get(id) { Some(name) => name, - None => return Ok(id.load(self.config)), + None => return Ok(id.load(self.config)?), }; let mut path = Path::new("/"); let orig_name = name; @@ -91,7 +91,7 @@ impl<'cfg> SourceConfigMap<'cfg> { name = s; path = p; } - None if *id == cfg.id => return Ok(id.load(self.config)), + None if *id == cfg.id => return Ok(id.load(self.config)?), None => { new_id = cfg.id.with_precise(id.precise() .map(|s| s.to_string())); @@ -105,8 +105,8 @@ impl<'cfg> SourceConfigMap<'cfg> { (configuration in `{}`)", name, path.display()) } } - let new_src = new_id.load(self.config); - let old_src = id.load(self.config); + let new_src = new_id.load(self.config)?; + let old_src = id.load(self.config)?; if new_src.supports_checksums() != old_src.supports_checksums() { let (supports, no_support) = if new_src.supports_checksums() { (name, orig_name) @@ -133,7 +133,7 @@ a lock file compatible with `{orig}` cannot be generated in this situation let mut srcs = Vec::new(); if let Some(val) = table.get("registry") { let url = url(val, &format!("source.{}.registry", name))?; - srcs.push(SourceId::for_registry(&url)); + srcs.push(SourceId::for_registry(&url)?); } if let Some(val) = table.get("local-registry") { let (s, path) = val.string(&format!("source.{}.local-registry", diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index 75c0281a4..ed156af83 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -25,18 +25,18 @@ pub struct GitSource<'cfg> { impl<'cfg> GitSource<'cfg> { pub fn new(source_id: &SourceId, - config: &'cfg Config) -> GitSource<'cfg> { + config: &'cfg Config) -> CargoResult> { assert!(source_id.is_git(), "id is not git, id={}", source_id); let remote = GitRemote::new(source_id.url()); - let ident = ident(source_id.url()); + let ident = ident(source_id.url())?; let reference = match source_id.precise() { Some(s) => GitReference::Rev(s.to_string()), None => source_id.git_reference().unwrap().clone(), }; - GitSource { + let source = GitSource { remote: remote, reference: reference, source_id: source_id.clone(), @@ -44,7 +44,9 @@ impl<'cfg> GitSource<'cfg> { rev: None, ident: ident, config: config, - } + }; + + Ok(source) } pub fn url(&self) -> &Url { self.remote.url() } @@ -57,8 +59,8 @@ impl<'cfg> GitSource<'cfg> { } } -fn ident(url: &Url) -> String { - let url = canonicalize_url(url); +fn ident(url: &Url) -> CargoResult { + let url = canonicalize_url(url)?; let ident = url.path_segments().and_then(|mut s| s.next_back()).unwrap_or(""); let ident = if ident == "" { @@ -67,13 +69,22 @@ fn ident(url: &Url) -> String { ident }; - format!("{}-{}", ident, short_hash(&url)) + Ok(format!("{}-{}", ident, short_hash(&url))) } // Some hacks and heuristics for making equivalent URLs hash the same -pub fn canonicalize_url(url: &Url) -> Url { +pub fn canonicalize_url(url: &Url) -> CargoResult { let mut url = url.clone(); + if url.cannot_be_a_base() { + if url.scheme() != "github.com" { + return Err(format!("invalid url `{}`: cannot-be-a-base-URLs are not supported", url).into()); + } + // it's most likely an attempt to fetch github repo + // https://github.com/rust-lang/cargo/issues/4394 + url = Url::parse(&format!("https://github.com/{}", url.path())).unwrap(); + } + // Strip a trailing slash if url.path().ends_with('/') { url.path_segments_mut().unwrap().pop_if_empty(); @@ -100,7 +111,7 @@ pub fn canonicalize_url(url: &Url) -> Url { url.path_segments_mut().unwrap().pop().push(&last); } - url + Ok(url) } impl<'cfg> Debug for GitSource<'cfg> { @@ -202,44 +213,57 @@ mod test { #[test] pub fn test_url_to_path_ident_with_path() { - let ident = ident(&url("https://github.com/carlhuda/cargo")); + let ident = ident(&url("https://github.com/carlhuda/cargo")).unwrap(); assert!(ident.starts_with("cargo-")); } #[test] pub fn test_url_to_path_ident_without_path() { - let ident = ident(&url("https://github.com")); + let ident = ident(&url("https://github.com")).unwrap(); assert!(ident.starts_with("_empty-")); } #[test] fn test_canonicalize_idents_by_stripping_trailing_url_slash() { - let ident1 = ident(&url("https://github.com/PistonDevelopers/piston/")); - let ident2 = ident(&url("https://github.com/PistonDevelopers/piston")); + let ident1 = ident(&url("https://github.com/PistonDevelopers/piston/")).unwrap(); + let ident2 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap(); assert_eq!(ident1, ident2); } #[test] fn test_canonicalize_idents_by_lowercasing_github_urls() { - let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")); - let ident2 = ident(&url("https://github.com/pistondevelopers/piston")); + let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap(); + let ident2 = ident(&url("https://github.com/pistondevelopers/piston")).unwrap(); assert_eq!(ident1, ident2); } #[test] fn test_canonicalize_idents_by_stripping_dot_git() { - let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")); - let ident2 = ident(&url("https://github.com/PistonDevelopers/piston.git")); + let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap(); + let ident2 = ident(&url("https://github.com/PistonDevelopers/piston.git")).unwrap(); assert_eq!(ident1, ident2); } #[test] fn test_canonicalize_idents_different_protocls() { - let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")); - let ident2 = ident(&url("git://github.com/PistonDevelopers/piston")); + let ident1 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap(); + let ident2 = ident(&url("git://github.com/PistonDevelopers/piston")).unwrap(); assert_eq!(ident1, ident2); } + #[test] + fn test_canonicalize_github_not_a_base_urls() { + let ident1 = ident(&url("github.com:PistonDevelopers/piston")).unwrap(); + let ident2 = ident(&url("https://github.com/PistonDevelopers/piston")).unwrap(); + assert_eq!(ident1, ident2); + } + + #[test] + fn test_canonicalize_not_a_base_urls() { + assert!(ident(&url("github.com:PistonDevelopers/piston")).is_ok()); + assert!(ident(&url("google.com:PistonDevelopers/piston")).is_err()); + } + fn url(s: &str) -> Url { s.to_url().unwrap() } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index fcf5a98d1..3b444d832 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -876,7 +876,7 @@ impl TomlDependency { .or_else(|| details.rev.clone().map(GitReference::Rev)) .unwrap_or_else(|| GitReference::Branch("master".to_string())); let loc = git.to_url()?; - SourceId::for_git(&loc, reference) + SourceId::for_git(&loc, reference)? }, (None, Some(path)) => { cx.nested_paths.push(PathBuf::from(path)); diff --git a/tests/resolve.rs b/tests/resolve.rs index 144fb608b..16395a255 100644 --- a/tests/resolve.rs +++ b/tests/resolve.rs @@ -44,7 +44,7 @@ trait ToDep { impl ToDep for &'static str { fn to_dep(self) -> Dependency { let url = "http://example.com".to_url().unwrap(); - let source_id = SourceId::for_registry(&url); + let source_id = SourceId::for_registry(&url).unwrap(); Dependency::parse_no_deprecated(self, Some("1.0.0"), &source_id).unwrap() } } @@ -86,7 +86,7 @@ macro_rules! pkg { fn registry_loc() -> SourceId { let remote = "http://example.com".to_url().unwrap(); - SourceId::for_registry(&remote) + SourceId::for_registry(&remote).unwrap() } fn pkg(name: &str) -> Summary { @@ -100,7 +100,7 @@ fn pkg_id(name: &str) -> PackageId { fn pkg_id_loc(name: &str, loc: &str) -> PackageId { let remote = loc.to_url(); let master = GitReference::Branch("master".to_string()); - let source_id = SourceId::for_git(&remote.unwrap(), master); + let source_id = SourceId::for_git(&remote.unwrap(), master).unwrap(); PackageId::new(name, "1.0.0", &source_id).unwrap() } @@ -112,14 +112,14 @@ fn pkg_loc(name: &str, loc: &str) -> Summary { fn dep(name: &str) -> Dependency { dep_req(name, "1.0.0") } fn dep_req(name: &str, req: &str) -> Dependency { let url = "http://example.com".to_url().unwrap(); - let source_id = SourceId::for_registry(&url); + let source_id = SourceId::for_registry(&url).unwrap(); Dependency::parse_no_deprecated(name, Some(req), &source_id).unwrap() } fn dep_loc(name: &str, location: &str) -> Dependency { let url = location.to_url().unwrap(); let master = GitReference::Branch("master".to_string()); - let source_id = SourceId::for_git(&url, master); + let source_id = SourceId::for_git(&url, master).unwrap(); Dependency::parse_no_deprecated(name, Some("1.0.0"), &source_id).unwrap() } fn dep_kind(name: &str, kind: Kind) -> Dependency { -- 2.30.2